Skip to content

Avoid more hygiene lookups #61484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 16 commits into from
Jun 5, 2019
Merged

Avoid more hygiene lookups #61484

merged 16 commits into from
Jun 5, 2019

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jun 3, 2019 •
edited
Loading

Mostly by combining multiple HygieneData::with calls into a single call on hot paths.

r? @petrochenkov

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Collaborator

bors commented Jun 3, 2019

⌛ Trying commit 485f83491d81f0de6ae3cc16f7c0a6126611a2d6 with merge 9840af41b95aedac3370bd7d4e663ee10d01be52...

@hellow554

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jun 3, 2019

☀️ Try build successful - checks-travis
Build commit: 9840af41b95aedac3370bd7d4e663ee10d01be52

@Centril
Copy link
Contributor

Centril commented Jun 3, 2019

@rust-timer build 9840af41b95aedac3370bd7d4e663ee10d01be52

@rust-timer
Copy link
Collaborator

Success: Queued 9840af41b95aedac3370bd7d4e663ee10d01be52 with parent c57ed9d, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9840af41b95aedac3370bd7d4e663ee10d01be52, comparison URL.

@nnethercote
Copy link
Contributor Author

Why all the XXX in the commit message? 😕 doesn't look nice

Because it's not finished. I filed the PR because I wanted to do a perf run on CI. That's why I did r? @ghost.

@nnethercote nnethercote force-pushed the avoid-more-hygiene-lookups branch from 485f834 to af026f6 Compare June 4, 2019 00:34
@nnethercote
Copy link
Contributor Author

I fixed up the commit messages and edited the PR description above.

r? @petrochenkov

@nnethercote nnethercote force-pushed the avoid-more-hygiene-lookups branch from af026f6 to 1d50c5f Compare June 4, 2019 03:54
petrochenkov
petrochenkov reviewed Jun 4, 2019
View reviewed changes
@petrochenkov
Copy link
Contributor

At this point (after introduction of all these combined methods) perhaps it makes sense to introduce hygiene::get_data() returning a short-lived reference to HygieneData (LocalInternedString-style) and allow other crates to combine operations themselves.

@petrochenkov
Copy link
Contributor

If HygieneData goes the same way as #59742, it will end up kept in Session with an optional TLS access when you really cannot pass the session.

hygiene::get_data() will be replaced with sess.hygiene_data in this case.

petrochenkov
petrochenkov reviewed Jun 4, 2019
View reviewed changes
@petrochenkov
Copy link
Contributor

Ok, there are 4 "obviously combined" operations right now - outer_is_descendant_of, modernize_and_adjust, outer_expn_info, outer_and_expn_info - not too bad.
I think we can live like this until HygieneData is moved into Session.

r=me with nits addressed

@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 4, 2019
nnethercote added 11 commits June 5, 2019 07:43
@nnethercote
Move modern calls inside glob_adjust and reverse_glob_adjust.
8797e8c
@nnethercote
Add some useful methods to HygieneData.
0ba36ea
@nnethercote
Add HygieneData::default_transparency.
7bec8c9 
Also use `HygieneData::expn_info` in an appropriate place.
@nnethercote
Add HygieneData::remove_mark.
cd64cc8
@nnethercote
Add HygieneData::adjust.
88fd7a8
@nnethercote
Add HygieneData::marks.
a02b2e3
@nnethercote
Add HygieneData::apply_mark_internal.
a84aee3
@nnethercote
Add HygieneData::apply_mark_with_transparency.
4527a86 
Also remove `HygieneData::apply_mark_internal`, which is no longer
needed.
@nnethercote
Add HygieneData::apply_mark.
58a4869 
This combines two `HygieneData::with` calls into one.
@nnethercote
Optimize glob_adjust and reverse_glob_adjust.
e19857c 
They can each now do a single `HygieneData::with` call by replacing the
`SyntaxContext` and `Mark` methods with the equivalent methods from
`HygieneData`.
@nnethercote
Add and use SyntaxContext::outer_and_expn_info.
f9209fc 
This combines two `HygieneData::with` calls into one on a hot path.
@nnethercote
Add SyntaxContext::hygienic_eq.
6596743 
This combines multiple `HygieneData::with` calls into one, by combining
parts of `hygienic_eq` and `adjust_ident`.
@nnethercote
Add walk_chain.
dc807a9 
This combines multiple `HygieneData::with` calls on a hot path.
@nnethercote
Add a useful comment about this file.
425736d
@nnethercote
Avoid unnecessary rust_2018 calls.
ab9bbf4 
The commit combines two calls into one by saving the result in a local
variable. The commit also moves the check for `async` later, so that
when a different keyword is present the `rust_2018` call will be avoided
completely.
@nnethercote
Copy link
Contributor Author

At this point (after introduction of all these combined methods) perhaps it makes sense to introduce hygiene::get_data() returning a short-lived reference to HygieneData (LocalInternedString-style) and allow other crates to combine operations themselves.

Exposing HygieneData::with would be safer. LocalInternedString's handling of lifetimes is quite dubious and not something we want to copy elsewhere, IMO.

@nnethercote
Add modernize_and_adjust methods.
4c9ecbf 
These combine two `HygieneData::with` calls into one.
@nnethercote nnethercote force-pushed the avoid-more-hygiene-lookups branch from 1d50c5f to 4c9ecbf Compare June 4, 2019 23:43
@nnethercote
Copy link
Contributor Author

I addressed the comments.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Jun 4, 2019

📌 Commit 4c9ecbf has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2019
@bors
Copy link
Collaborator

bors commented Jun 5, 2019

⌛ Testing commit 4c9ecbf with merge 2a1d6c8...

bors added a commit that referenced this pull request Jun 5, 2019
@bors
Auto merge of #61484 - nnethercote:avoid-more-hygiene-lookups, r=petr…
2a1d6c8 
…ochenkov

Avoid more hygiene lookups

Mostly by combining multiple `HygieneData::with` calls into a single call on hot paths.

r? @petrochenkov
@bors
Copy link
Collaborator

bors commented Jun 5, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing 2a1d6c8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 5, 2019
@bors bors merged commit 4c9ecbf into rust-lang:master Jun 5, 2019
@nnethercote nnethercote deleted the avoid-more-hygiene-lookups branch June 11, 2019 19:43
@nnethercote nnethercote mentioned this pull request Jun 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @bors @hellow554 @Centril @rust-timer @petrochenkov